Let a file type declare which lines can carry a breakpoint and BreakPointStore is now a Singleton per File - #357
Draft
danielpourbakhsh wants to merge 3 commits into
Draft
Conversation
The breakpoint margin occupied a column of its own between the line numbers and the text. Rider and VS Code instead put the breakpoint on the line number: the number gives way to the dot when one is set, and the gutter keeps the width the numbers give it. BreakPointLineNumberMargin derives from LineNumberMargin and takes over its slot in TextArea.LeftMargins, so MeasureOverride stays inherited and the column is exactly as wide as it was. A click there now means "breakpoint" and nothing else - the base class's line selection is deliberately skipped, matching what those editors do. BreakPointMargin is marked obsolete rather than removed, so anyone using it directly keeps working. Two details that are not obvious from the diff: the foreground colour is read from the editor because AvaloniaEdit binds LineNumbersForeground only on the margin it creates itself, and the dot shrinks when it would not fit, so the column never grows wider than the numbers alone would make it.
SetEnableBreakpoints gave every editor its own BreakpointStore, so a breakpoint reached nothing beyond the margin that drew it: closing and reopening the file lost it, the same file open in two views held two unrelated sets, and no debugger could read any of them. BreakpointStore.Instance is now that one store, following ExplorerNameComparer and TypeAssistanceIconStore in the same assembly. A shared store outlives the margins that use it, so both margins now subscribe for as long as they are attached instead of from their constructor on. Subscribing once and never detaching would let the store hold every margin of every closed editor alive and redraw them on each change - harmless while the store died with the margin, a leak once it does not.
The margin accepts a breakpoint on every line of a file whose type supports them. A language whose lines are not all executable has no way to say so, and a breakpoint on such a line does not fail visibly: the backend moves it to the next line that has code, silently, while the dot stays where the user put it. ITypeAssistance gains BreakPointLinePattern, a default interface member returning null, so every existing language keeps its current behaviour and no implementer has to change. The rule stays with the file type; the margin only applies it. Checking the line text rather than the line number keeps any one language's syntax out of the core. An invalid pattern from a plugin is logged once and then treated as no restriction, and removing a breakpoint always stays possible, so one that predates a rule change can still be taken away. The hover preview follows the same rule, since a dot that disappears on release is the most misleading feedback available. SetEnableBreakpoints now takes the ITypeAssistance rather than a flag, since the margin needs both values as a unit. The margin's new parameter is optional, so existing callers keep working unchanged. Not included: an upper limit per file. A target's breakpoint capacity is a property of the target, not of the file type, and it is a per-target resource that cannot be counted per file. IDebugSession.SetBreakpointAsync already reports whether the target took the breakpoint, which is where that belongs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Three things about breakpoints in the editor, one commit each. 7 files,
+258 / −7.
it's own. -> Inspired by the UIs of popular and well-kown IDEs
that set it. -> Fixes Bug Scenario: Same File open in two Editor Instances, with two asynchronous Breakpoint Stores.
1. Breakpoints on the line numbers
The breakpoint margin sat between the line numbers and the text, costing a
column. Rider and VS Code instead put the breakpoint on the line number itself:
the number gives way to the dot, and the gutter keeps the width the numbers give
it.
BreakPointLineNumberMarginderives fromLineNumberMarginand takes over itsslot in
TextArea.LeftMargins, soMeasureOverridestays inherited and thecolumn is exactly as wide as before.
Behaviour change: in a file whose type supports breakpoints, a click on the
line number now toggles a breakpoint and no longer selects the line. That is
what those editors do, and the two meanings cannot share one click. Files whose
type does not support breakpoints keep the stock margin and its selection
behaviour untouched.
BreakPointMarginis marked[Obsolete]rather than removed, so anyone usingit directly keeps working.
Two details that are not obvious from the diff: the foreground colour is read
from the editor, because AvaloniaEdit binds
LineNumbersForegroundonly on themargin it creates itself and not on one inserted in its place; and the dot
shrinks when it would not fit, so the column never grows wider than the numbers
alone would make it.
2. One shared breakpoint store
SetEnableBreakpointsgave every editor its ownBreakpointStore, so abreakpoint reached nothing beyond the margin that drew it.
BreakpointStore.Instanceis now that one store, following
ExplorerNameComparerandTypeAssistanceIconStorein the same assembly.Behaviour changes, both intended:
A shared store outlives the margins that use it, so both margins now subscribe
for as long as they are attached rather than from their constructor on.
Subscribing once and never detaching would let the store hold every margin of
every closed editor alive and redraw them on each change — harmless while the
store died with the margin, a leak once it does not. The obsolete margin gets
the same treatment, since it is still constructible.
3. A file type declares its breakpointable lines
The margin accepts a breakpoint on every line of a file whose type supports
them. For a language where not every line is executable — a comment, a blank
line, a directive — the breakpoint does not fail visibly. The backend moves it
to the next line that has code, silently, while the dot stays where the user put
it. The dot and the place the target actually halts are then two different lines,
and nothing says so.
ITypeAssistancegains one member:A default interface member returning
null, so every existing language keepsits current behaviour and no implementer has to change.
TypeAssistanceBaseexposes it as a
protected initproperty, next toLineCommentSequence.The rule stays with the file type; the margin only applies it. The check reads
the line text, not the line number, which keeps any one language's syntax out
of the core.
Two questions I would like your read on
Opened as a draft because of these. The code works and builds; both questions
are about where things belong, and you know this codebase and its plugins
better than I do.
1. Where should a target's breakpoint capacity live?
The case that prompted this: the target I debug holds 16 breakpoints in
hardware — a fixed number of comparators. Today the margin accepts any number
of them, and the seventeenth fails when the session starts, far from the click
that caused it.
What I tried first, and dropped. A
MaxBreakPointsmember next toBreakPointLinePatternonITypeAssistance. It works, and I removed it again,because it does not survive its own reasoning:
source can run on targets with different capacities. Putting the number behind
a language interface means reaching a hardware constant through the wrong
contract.
ITypeAssistanceis per file type and themargin counts per file. Two source files on one target, and the count is wrong.
So the pattern in this PR is a language property and stays; the capacity is not
one and did not.
The direction I would take instead, which needs no new surface:
IDebugSession.SetBreakpointAsyncalready returns whether the target acceptedthe breakpoint, and today nothing looks at that value. A caller could mark a
refused breakpoint unverified and the margin draw it hollow — how VS Code and
Visual Studio show a breakpoint the target would not take. That covers every
reason for a refusal, not just capacity: an address that cannot be mapped, code
that is not loaded, a comparator already in use.
What I am asking:
a reason to know the limit up front?
the active target? I did not find one, and I would rather use yours than invent
a second.
I have deliberately left this out of this PR either way — it needs a debugger to
produce the answer, and there is none in this repository yet.
2. How far does replacing
BreakPointMarginreach?BreakPointLineNumberMarginfully supersedesBreakPointMargin, and after thisPR nothing in this repository constructs the old one. I marked it
[Obsolete]rather than deleting it.The reason I did not delete it is that I cannot see far enough. It has been
publicinOneWare.Essentialssince July 2024 and shipped in 68 taggedreleases. Whether a plugin out there builds its own gutter with it is not
something I can determine from here.
What I am asking: can you tell whether anything outside this repository uses
it? If you are confident nobody does, I will delete it in this PR — it is one
commit either way, and 150 lines of code that nothing calls will not age well.
The same question applies in smaller form to the one signature this PR does
change,
ExtendedTextEditor.SetEnableBreakpoints. I am happy to keep thebooloverload alongside the new one if you would rather not break it.